opt: map and push down equality conditions #41250
Conversation
|
This change is |
There was a problem hiding this comment.
Nice! 
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):
for i := range filters { fd := &filters[i].ScalarProps(c.mem).FuncDeps filterEqCols := fd.ComputeEquivClosure(fd.EquivReps())
Is there a case today in which this does not correspond to just a pair of columns being equated?
pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):
filters memo.FiltersExpr, src *memo.FiltersItem, dst memo.RelExpr, ) opt.ScalarExpr { // Fast path if src is already bound by dst.
can this be easily unified with CanMapJoinOpEquality?
pkg/sql/opt/norm/join.go, line 366 at r1 (raw file):
if !scalarProps.OuterCols.Contains(col) { // Filter does not contain col. eqCols = scalarProps.FuncDeps.ComputeEquivClosure(eqCols)
Can't this still miss some, if we have a case like:
col: x
filter: x = y AND w = z AND y = w
?
(assuming I understood the problem that prompted this change correctly)
There was a problem hiding this comment.
oops, only looked at first commit
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/join.go, line 331 at r2 (raw file):
(newFilters == nil && c.CanMapJoinOpEquality(filters, filter, dst)) { if newFilters == nil { // All right, it was necessary to allocate the filters after all.
lol, I like the resigned tone of this comment
pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):
} newFilters[i] = memo.FiltersItem{
Maybe I'm missing some important thing here, is it not possible for the replacement of the filter like this to cause us to lose some information? Like, I get if we were just appending this newly mapped filter we'd be fine, but couldn't replacing like this result in making the replacement no longer valid (since we use the filters themselves to justify the mapping)?
Like if we had something like:
a = b AND b = c AND c = d AND d = e AND e = f AND f = a
(a 6-cycle)
and then we replaced b = c => b = f and e = f => e = c, we no longer have an equivalent set of filters, we have two equivalence groups ({a,b,f} and {c,d,e}) rather than one ({a,b,c,d,e,f}).
There was a problem hiding this comment.
TFTR! Very thoughtful comments.
Reviewable status:
pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Is there a case today in which this does not correspond to just a pair of columns being equated?
I don't think so -- in logical_props_builder.go we only add equivalencies for filters that are variable equalities.
I did it this way since it seemed like the easiest way to extract the equivalent columns and remove them from eqCols.
pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
can this be easily unified with
CanMapJoinOpEquality?
Not sure if I understand the question, but are you asking whether we can get rid of the fast path here by returning false from CanMapJoinOpEquality?
The problem is that we need CanMapJoinOpEquality to return true for this fast path so that the rule PushEqualityIntoJoinLeftAndRight will fire when appropriate. For example, suppose we have the following query: SELECT * from a, b WHERE a.x = a.y AND a.x = b.x AND a.y = b.y;. PushEqualityIntoJoinLeftAndRight has to fire to prevent MapEqualityIntoJoinLeft and MapEqualityIntoJoinRight from cycling with each other. They will infinitely loop mapping a.x=a.y to b.x=b.y and vice-versa.
I added a note above PushEqualityIntoJoinLeftAndRight to emphasize that it has to be first (this is the same pattern we used for the existing filter push down rules).
pkg/sql/opt/norm/join.go, line 366 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Can't this still miss some, if we have a case like:
col: x filter: x = y AND w = z AND y = w?
(assuming I understood the problem that prompted this change correctly)
Good point! Fixed.
pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Maybe I'm missing some important thing here, is it not possible for the replacement of the filter like this to cause us to lose some information? Like, I get if we were just appending this newly mapped filter we'd be fine, but couldn't replacing like this result in making the replacement no longer valid (since we use the filters themselves to justify the mapping)?
Like if we had something like:
a = b AND b = c AND c = d AND d = e AND e = f AND f = a(a 6-cycle)
and then we replacedb = c=>b = fande = f=>e = c, we no longer have an equivalent set of filters, we have two equivalence groups ({a,b,f}and{c,d,e}) rather than one ({a,b,c,d,e,f}).
This situation is not possible because of the blocks in CanMapJoinOpEquality and MapJoinOpEquality that have the following comment:
// Remove from consideration equality columns that already have an equality
// condition bound by dst.
So in the example you gave, suppose that the columns in dst are {a,b,f}, and we want to try to map b=c to use {a,b,f}. Since a=b and f=a are filters already bound by dst, all three columns (a, b and f) will be removed from consideration for the new equality condition. Therefore, it won't be possible to map to b=f.
That being said, I agree there's something about this that feels slightly weird. Another way to think about this problem is that if every column in the equivalence group is a node in a graph, we want to find the minimum spanning tree for the graph, where edges between nodes in dst have lower weight. I'm not sure if formulating it in this way makes it easier/cleaner to solve, but I'll spend some time thinking about it.
One thing I do like about this solution, though, is that it reuses the code from the normalization rules, and the normalization rules follow the same pattern we already used for filter push down.
Anyway, let me know if you have an idea about how to make this better!
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I don't think so -- in
logical_props_builder.gowe only add equivalencies for filters that are variable equalities.I did it this way since it seemed like the easiest way to extract the equivalent columns and remove them from
eqCols.
Makes sense! Just wanted to make sure I was understanding right
pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Not sure if I understand the question, but are you asking whether we can get rid of the fast path here by returning false from
CanMapJoinOpEquality?The problem is that we need
CanMapJoinOpEqualityto return true for this fast path so that the rulePushEqualityIntoJoinLeftAndRightwill fire when appropriate. For example, suppose we have the following query:SELECT * from a, b WHERE a.x = a.y AND a.x = b.x AND a.y = b.y;.PushEqualityIntoJoinLeftAndRighthas to fire to preventMapEqualityIntoJoinLeftandMapEqualityIntoJoinRightfrom cycling with each other. They will infinitely loop mappinga.x=a.ytob.x=b.yand vice-versa.I added a note above
PushEqualityIntoJoinLeftAndRightto emphasize that it has to be first (this is the same pattern we used for the existing filter push down rules).
Sorry, was a little too terse, I meant these couple blocks of code seem very similar to the blocks in CanMapJoinOpEquality and it feels like maybe there's an extraction possible. If not that's fine!
pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
This situation is not possible because of the blocks in
CanMapJoinOpEqualityandMapJoinOpEqualitythat have the following comment:// Remove from consideration equality columns that already have an equality // condition bound by dst.So in the example you gave, suppose that the columns in
dstare{a,b,f}, and we want to try to mapb=cto use{a,b,f}. Sincea=bandf=aare filters already bound bydst, all three columns (a, b and f) will be removed from consideration for the new equality condition. Therefore, it won't be possible to map tob=f.That being said, I agree there's something about this that feels slightly weird. Another way to think about this problem is that if every column in the equivalence group is a node in a graph, we want to find the minimum spanning tree for the graph, where edges between nodes in
dsthave lower weight. I'm not sure if formulating it in this way makes it easier/cleaner to solve, but I'll spend some time thinking about it.One thing I do like about this solution, though, is that it reuses the code from the normalization rules, and the normalization rules follow the same pattern we already used for filter push down.
Anyway, let me know if you have an idea about how to make this better!
Talked about this offline
There was a problem hiding this comment.
I've completely changed this to use a minimum-spanning tree approach as Justin and I discussed offline. PTAL!
Reviewable status:
There was a problem hiding this comment.
Very cool! Some minor comments.
Reviewable status:
pkg/sql/opt/norm/join.go, line 122 at r3 (raw file):
// // If src has a correlated subquery, CanMapJoinOpFilter returns false. func (c *CustomFuncs) CanMapJoinOpFilter(
we should move this so it stays close to MapJoinOpFilter
pkg/sql/opt/norm/join.go, line 171 at r3 (raw file):
// If more than one equality condition connecting columns in the equivalence // group spans both sides of the join, these conditions can be remapped. found := false
[nit] would be more suggestive if we used found := 0 and then found++; if found > 1 { return true }
pkg/sql/opt/norm/join.go, line 195 at r3 (raw file):
// containing col that forms an equivalence group in filters. Then it // (conceptually) constructs a graph in which the nodes represent columns and // the edges represent equality conditions between the columns. The goal is to
This is a bit confusing. There is the graph that represents the given equality conditions (the first diagram below), and then there's the conceptual complete graph where there's an edge between any pair of columns in the equivalency group. We are talking about a spanning tree of the latter, not the former. The former graph isn't important really, I don't think we should draw it.
What does "minimum" spanning tree mean here? Sounds like we're looking for any spanning tree with a single "crossing" edge. I get that the "restriction" can be thought of as a per-edge cost but we should use either one definition or the other.
We may be complicating things unnecessarily by talking about graphs and spanning trees. We could just say that we have a set of columns that are all equivalent, some on the left side and some on the right side. We construct a new set of equalities that implies the same equivalency group, with the property that there is a single condition with one left column and one right column.
pkg/sql/opt/norm/join.go, line 246 at r3 (raw file):
)) } for col, ok := leftEqCols.Next(prevCol + 1); ok; col, ok = leftEqCols.Next(col + 1) {
Is it important here how we "connect" them? It would be more intuitive (and easier to descripbe) to just pick an arbitrary column on each side and make everything on that side equal to that column. Eg if we have l1, l2, l3 and r1, r2, r3, we could pick l1 and r2 and generate l1 = r2 plus l1 = l2, l1 = l3 plus r1 = r2, r1 = r3.
pkg/sql/opt/norm/join.go, line 252 at r3 (raw file):
prevCol = col } for col, ok := rightEqCols.Next(0); ok; col, ok = rightEqCols.Next(col + 1) {
It's subtle that the "crossing" condition happens here implicitly, should add a comment.
There was a problem hiding this comment.
TFTR! Updated.
Reviewable status:
pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Sorry, was a little too terse, I meant these couple blocks of code seem very similar to the blocks in
CanMapJoinOpEqualityand it feels like maybe there's an extraction possible. If not that's fine!
This code is different now.
pkg/sql/opt/norm/join.go, line 122 at r3 (raw file):
Previously, RaduBerinde wrote…
we should move this so it stays close to
MapJoinOpFilter
Done.
pkg/sql/opt/norm/join.go, line 171 at r3 (raw file):
Previously, RaduBerinde wrote…
[nit] would be more suggestive if we used
found := 0and thenfound++; if found > 1 { return true }
Done.
pkg/sql/opt/norm/join.go, line 195 at r3 (raw file):
Previously, RaduBerinde wrote…
This is a bit confusing. There is the graph that represents the given equality conditions (the first diagram below), and then there's the conceptual complete graph where there's an edge between any pair of columns in the equivalency group. We are talking about a spanning tree of the latter, not the former. The former graph isn't important really, I don't think we should draw it.
What does "minimum" spanning tree mean here? Sounds like we're looking for any spanning tree with a single "crossing" edge. I get that the "restriction" can be thought of as a per-edge cost but we should use either one definition or the other.
We may be complicating things unnecessarily by talking about graphs and spanning trees. We could just say that we have a set of columns that are all equivalent, some on the left side and some on the right side. We construct a new set of equalities that implies the same equivalency group, with the property that there is a single condition with one left column and one right column.
Done.
pkg/sql/opt/norm/join.go, line 246 at r3 (raw file):
Previously, RaduBerinde wrote…
Is it important here how we "connect" them? It would be more intuitive (and easier to descripbe) to just pick an arbitrary column on each side and make everything on that side equal to that column. Eg if we have
l1, l2, l3andr1, r2, r3, we could pickl1andr2and generatel1 = r2plusl1 = l2,l1 = l3plusr1 = r2,r1 = r3.
Done.
pkg/sql/opt/norm/join.go, line 252 at r3 (raw file):
Previously, RaduBerinde wrote…
It's subtle that the "crossing" condition happens here implicitly, should add a comment.
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/join.go, line 163 at r4 (raw file):
// SELECT * FROM a, b WHERE a.x = a.y AND b.x = b.y AND a.x = b.x // func (c *CustomFuncs) MapJoinOpEqualities(
Nice, this function is super clear now 👍
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/join.opt, line 179 at r4 (raw file):
... $item:(FiltersItem (Eq (Variable $col:*) (Variable))) & (CanMapJoinOpEqualities $on $col $leftCols:(OutputCols $left) $rightCols:(OutputCols $right))
If we have a lot of equalities, and we can't Map them, we will recompute this function for each item no? Could we just check it once on the entire $on? (I'd think the common case is to have at least one equality)
…rule This commit adds a new normalization rule to enable pushing variable equality conditions such as a.x=b.x through joins. For example, consider this query: SELECT * FROM a, b, c WHERE a.x=b.x AND a.x=c.x Given join ordering (a join (b join c)), it should be possible to infer the filter b.x=c.x and push it down from the top level onto the join (b join c). This commit enables that mapping and pushdown to happen. In addition, this commit updates the AssociateJoin rule to map as many equality conditions as possible to use the output columns of the new inner-most join, allowing those conditions to be pushed onto that join. For example, consider this query: SELECT * FROM a, b, c WHERE a.x=b.x AND b.x=c.x If the AssociateJoin rule creates a new join ordering (b join (a join c)), it should be possible to map a.x=b.x to a.x=c.x and add it onto the new inner-most join (a join c). This commit enables that mapping to happen. Release note (performance improvement): Improved performance for some join queries due to improved filter inference during planning. Release justification: This commit will not be merged before the release branch is cut.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/join.opt, line 179 at r4 (raw file):
Previously, RaduBerinde wrote…
If we have a lot of equalities, and we can't Map them, we will recompute this function for each item no? Could we just check it once on the entire
$on? (I'd think the common case is to have at least one equality)
Good point -- updated so now it just checks once per equivalence group rather than once per equality condition.
There was a problem hiding this comment.
bors r+
Reviewable status:
Build failed (retrying...) |
41010: roachtest: remove wait loop in backup2TB roachtest r=pbardea a=pbardea Previously a wait loop was needed in the backup2TB roachtest because the test was reporting the table as offline when it shouldn't have seen it as OFFLINE. This was fixed by #40996, and therefore we should no longer need this wait loop. Closes #36841. Release justification: Only touches tests. Release note: None 41250: opt: map and push down equality conditions r=rytaft a=rytaft This commit adds a new normalization rule to enable pushing variable equality conditions such as `a.x=b.x` through joins. For example, consider this query: `SELECT * FROM a, b, c WHERE a.x=b.x AND a.x=c.x` Given join ordering `(a join (b join c))`, it should be possible to infer the filter `b.x=c.x` and push it down from the top level onto the join `(b join c)`. This commit enables that mapping and pushdown to happen. In addition, this commit updates the `AssociateJoin` rule to map as many equality conditions as possible to use the output columns of the new inner-most join, allowing those conditions to be pushed onto that join. For example, consider this query: `SELECT * FROM a, b, c WHERE a.x=b.x AND b.x=c.x` If the AssociateJoin rule creates a new join ordering `(b join (a join c))`, it should be possible to map `a.x=b.x` to `a.x=c.x` and add it onto the new inner-most join `(a join c)`. This commit enables that mapping to happen. Fixes #38716 Fixes #36226 Release note (performance improvement): Improved performance for some join queries due to improved filter inference during planning. Release justification: This commit will not be merged before the release branch is cut. Co-authored-by: Paul Bardea <pbardea@gmail.com> Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Build succeeded |
This commit adds a new normalization rule to enable pushing variable
equality conditions such as
a.x=b.xthrough joins.For example, consider this query:
SELECT * FROM a, b, c WHERE a.x=b.x AND a.x=c.xGiven join ordering
(a join (b join c)), it should be possible to infer thefilter
b.x=c.xand push it down from the top level onto the join(b join c).This commit enables that mapping and pushdown to happen.
In addition, this commit updates the
AssociateJoinrule to map as manyequality conditions as possible to use the output columns of the new inner-most
join, allowing those conditions to be pushed onto that join.
For example, consider this query:
SELECT * FROM a, b, c WHERE a.x=b.x AND b.x=c.xIf the AssociateJoin rule creates a new join ordering
(b join (a join c)),it should be possible to map
a.x=b.xtoa.x=c.xand add it onto the newinner-most join
(a join c). This commit enables that mapping to happen.Fixes #38716
Fixes #36226
Release note (performance improvement): Improved performance for some join
queries due to improved filter inference during planning.
Release justification: This commit will not be merged before the release
branch is cut.